Skip to content

Comments

feat: Bring back cassandra adapter using gocqlx v2#138

Open
ayashjorden wants to merge 3 commits intomainfrom
feat/cassandra_use_gocqlx_v2
Open

feat: Bring back cassandra adapter using gocqlx v2#138
ayashjorden wants to merge 3 commits intomainfrom
feat/cassandra_use_gocqlx_v2

Conversation

@ayashjorden
Copy link
Contributor

Use SHA for github actions to avoid issues with
caching and to ensure that the correct version of
the action is used.

@ayashjorden ayashjorden force-pushed the feat/cassandra_use_gocqlx_v2 branch 3 times, most recently from c0eca16 to ab098e3 Compare February 8, 2026 03:17
@ayashjorden ayashjorden force-pushed the feat/cassandra_use_gocqlx_v2 branch from ab098e3 to 660546c Compare February 8, 2026 03:23
@ayashjorden ayashjorden force-pushed the feat/cassandra_use_gocqlx_v2 branch from 828c390 to e796686 Compare February 16, 2026 08:12
Use SHA for github actions to avoid issues with
caching and to ensure that the correct version of
the action is used.
@ayashjorden ayashjorden force-pushed the feat/cassandra_use_gocqlx_v2 branch 2 times, most recently from 1adf4c3 to f93c21b Compare February 16, 2026 08:16
@ayashjorden ayashjorden force-pushed the feat/cassandra_use_gocqlx_v2 branch from f93c21b to 7bc3587 Compare February 16, 2026 08:27
Copy link
Contributor

@deanefrati deanefrati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments from my AI assisted initial review, i still need to go through it in mode detail myself and check it in a few of my micro-services to make sure it doesn't break anything which i will do in the next couple of days.

Design Review: Cassandra Adapter Implementation

Overview

This PR reintroduces the Cassandra adapter using gocqlx/v2. The implementation correctly follows the storage adapter interface and integrates well with the existing codebase. However, the design differs from other adapters in several ways - some are intentional and appropriate for Cassandra/gocqlx, while others should be addressed for consistency.


Design Consistency Analysis

✅ Strengths

  1. Adapter Interface Compliance: Correctly implements all required StorageAdapter methods
  2. Singleton Pattern: Consistent with other adapters (SQL, DynamoDB, CosmosDB)
  3. Error Handling: Uses errors.Join for proper error wrapping
  4. Table Metadata: Pre-loads table metadata using gocqlx, which is appropriate for Cassandra
  5. Session Management: On-demand session creation with proper cleanup (defer s.Close()) is reasonable for Cassandra

🔄 Design Differences (Intentional vs. Inconsistencies)

1. Initialization Pattern

  • Current: Inline initialization in GetCassandraAdapter() with error return
  • Other Adapters: Separate OpenConnection() method, no error return
  • Impact: API inconsistency - Cassandra is the only adapter that can fail during factory creation
  • Recommendation: Consider standardizing factory method signature across adapters, or document why Cassandra differs

2. Connection Management

  • Current: On-demand session creation per operation
  • Other Adapters: Persistent connections/clients stored in struct
  • Impact: Different but acceptable for Cassandra; gocql.ClusterConfig manages connection pool
  • Status: ✅ Acceptable design choice

3. Table Metadata Pre-loading

  • Current: Pre-loads all table metadata in initializeTableMappers() during initialization
  • Other Adapters: Compute table names dynamically from type names
  • Impact: Requires keyspace and tables to exist at initialization time
  • Status: ✅ Appropriate for gocqlx, but less flexible than other adapters

🐛 Issues Found

Critical Bugs

1. Migration Table Handling (Lines 236, 261)

t, e := c.getTableForItem("migrations")  // ❌ Won't work
  • Problem: getTableForItem() expects a struct type, not a string. Passing "migrations" will fail because typeName("migrations") returns empty string.
  • Fix: Use direct table lookup: c.tables["migrations"] or create a migration struct type

2. Count Method Returns Wrong Value (Line 395)

return -1, t.SelectBuilder().CountAll().Query(s).BindStruct(dest).ExecRelease()
  • Problem: Always returns -1 instead of actual count. The count result is not captured.
  • Fix: Capture and return the count value from the query result

3. Wrong Error Variable (Line 272)

return -1, errors.Join(
    fmt.Errorf("failed creating a session"),
    e,  // Should be sErr
)
  • Problem: Uses e (from line 261) instead of sErr (from line 268)
  • Fix: Change e to sErr

Minor Issues

4. Typo in storage/reflection.go:20

  • "fot" should be "for"

5. Max("id") Usage (Line 276)

  • Verify that gocqlx/v2 SelectBuilder supports Max() method. May need raw CQL query instead.

💡 Recommendations

High Priority

  1. ✅ Fix migration table handling - use direct table lookup or migration struct
  2. ✅ Fix Count method - return actual count value
  3. ✅ Fix error variable bug in GetLatestMigration()

Medium Priority

  1. ⚠️ Consider API consistency - either:
    • Make all factory methods return (Adapter, error), OR
    • Make Cassandra's factory method match others (no error return, handle errors differently)
  2. ⚠️ Add unit tests for Cassandra adapter methods, especially edge cases

Low Priority

  1. 📝 Fix typo in reflection.go
  2. 📝 Verify Max() method usage or use alternative approach for GetLatestMigration()
  3. 📝 Consider adding connection pooling documentation explaining why sessions are created per-operation

📊 Overall Assessment

The implementation is solid and follows gocqlx patterns correctly. The main concerns are:

  • Three bugs that need fixing (migration table, Count method, error variable)
  • API inconsistency in factory method signature
  • Some design differences that are acceptable but should be documented

Recommendation: Address the critical bugs before merging. The API consistency issue can be handled in a follow-up if needed, but documenting the differences would be helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants